Keep a FOS report readable after its task is gone - #1237
Merged
Conversation
Port of 1.6's schema 341 (#1236). The writer half only: this branch has no Task Management log pane, so there is no reader to change -- which is also what makes the defect worse here rather than milder. taskLog stores no host and no task type of its own and reaches both through `tasks`. Nothing deletes taskLog rows -- but Host::destroy() calls TaskManager->destroy() and taskLog is in no cascade at all, so deleting a host destroys its tasks and leaves the reports behind with nothing to join to. The host name goes at the same moment the host row that could supply it does. On 1.6 the log pane at least still shows the text. Here the REST API is the only reader `tasklog` has, and it hands back a report whose taskID points at nothing, with no way at all to learn which machine it came from. The point of GH-1206 is that a failure message is findable later instead of arriving as a phone photo of a wrapped console, and a foreign key to a routinely-deleted row cannot deliver that. Blocking deletion of a task that has reports was the alternative. It inverts the dependency -- a diagnostic artifact would then constrain operational cleanup -- and to be consistent it would have to block HOST deletion too, since that is the path that actually removes tasks. Refusing to delete a host because it once failed to image is a worse product than losing a host name. Schema 283 adds logHostID, logHostName and logTaskTypeName, backfilling the reports whose task is still there so the history is not split between rows that can answer and rows that cannot. TaskError fills them; nothing else does. Every other row here is a state transition written by TaskingElement::taskLog() on every transition, meaningless without its task anyway -- the same reasoning that gave logText no value on a state row in step 280. The state a row records was never at risk: taskLog stores taskStateID itself, so that lookup survives its task. Two column shapes, following what the writer can produce: save() omits an unset OPTIONAL column whose key ends in "id", so logHostID lands NULL, but writes '' for every other unset field -- the trap step 282 had to repair for logType. Declaring logHostName NOT NULL DEFAULT '' says what will really be stored. Not ported: the manifest repair that was the other half of #1236. This branch has no schema-expected.php, no SchemaReconciler and no bin/schema-manifest.php, so there is no second description of taskLog to drift out of step. tests/task-error-report.test.php gains four checks in its own idiom -- the writer stores each field, the schema step adds the columns, and it backfills. Mutation-verified: dropping the three set() calls, dropping a field from the model, misspelling a column in the ALTER and removing the backfill each fail it. Step 283 was executed against a 1.5 server over temporary shadow tables and produces the intended rows. sh tests/run-all.sh -- 19 passed, 0 failed. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Port of 1.6's schema 341 (#1236). Writer half only — this branch has no Task Management log pane, so there is no reader to change, which is also what makes the defect worse here rather than milder.
The problem
taskLogstores no host and no task type of its own and reaches both throughtasks. Nothing deletestaskLogrows — butHost::destroy()callsTaskManager->destroy()andtaskLogis in no cascade at all. So deleting a host destroys its tasks and leaves the reports behind with nothing to join to, losing the host name at the same moment the host row that could have supplied it goes.On 1.6 the log pane at least still shows the text with a blank host column. Here
tasklogis inRoute::$validClassesand the REST API is its only reader — it hands back a report whosetaskIDpoints at nothing and no way at all to learn which machine it came from.The fix
Schema 283 adds
logHostID,logHostName,logTaskTypeNameand backfills the reports whose task is still there, so the history is not split between rows that can answer the question and rows that cannot.TaskErrorfills them; nothing else does.Every other row in this table is a state transition written by
TaskingElement::taskLog()on every transition — meaningless without its task anyway, so it leaves them empty. Same reasoning that gavelogTextno value on a state row in step 280.The state a row records was never at risk:
taskLogstorestaskStateIDitself, so that lookup survives its task.Two column shapes, following what the writer can actually produce:
save()omits an unset optional column whose key ends inid, sologHostIDlands NULL, but writes''for every other unset field — the trap step 282 had to repair forlogType.Why not block deleting a task that has reports: it inverts the dependency — a diagnostic artifact would constrain operational cleanup — and to be consistent it would have to block host deletion too, since that's the path that actually removes tasks.
Not ported
The manifest repair that was the other half of #1236. This branch has no
schema-expected.php, noSchemaReconcilerand nobin/schema-manifest.php, so there is no second description oftaskLogto drift out of step.Tests
tests/task-error-report.test.phpgains four checks in its own source-level idiom: the writer stores each field, the schema step adds the columns, and it backfills. Mutation-verified — dropping the threeset()calls, dropping a field from the model, misspelling a column in theALTER, and removing the backfill each fail it.Step 283 was executed against a 1.5 server over temporary shadow tables and produces the intended rows: report with a live task filled, report already orphaned left empty, state row untouched.
sh tests/run-all.sh— 19 passed, 0 failed.Downstream
No route classes changed, so no FogApi sync is implied.